[SPARK-57847][SQL] Support the TIME data type in approx_count_distinct_for_intervals#56934
[SPARK-57847][SQL] Support the TIME data type in approx_count_distinct_for_intervals#56934yadavay-amzn wants to merge 2 commits into
Conversation
…nt_distinct_for_intervals test
MaxGekk
left a comment
MaxGekk
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
Clean, correct extension of approx_count_distinct_for_intervals to TIME. TIME is PhysicalLongType (nanos-of-day), so it routes through the same Long -> Double bucketing path as timestamp/interval; the three accept/convert sites are updated in lockstep (the toDoubleValue match has no catch-all, so a missed site would have thrown a MatchError), the hash path handles TimeType, and tests cover both small and full-range nanos plus the updated error messages. Follows the SPARK-57557 precedent.
Verification
Traced all three type paths for TIME: analysis accept (inputTypes + endpoint-array check), runtime child conversion (asInstanceOf[Long].toDouble) and endpoint conversion (getAccessor -> getLong -> .toString.toDouble), and the downstream XxHash64 (case LongType | _: TimeType => genHashLong). All operate on the same nanos-of-day Long domain, so TIME values and endpoints bucket consistently — identical to the existing timestamp/interval types.
…t_for_intervals ### What changes were proposed in this pull request? Adds `TimeType` to the input types accepted by the `approx_count_distinct_for_intervals` aggregate. TIME values are bucketed by their internal nanosecond-of-day `Long` representation, routed through the same `Long -> Double` path already used for `TimestampType` / `DayTimeIntervalType`. ### Why are the changes needed? `approx_count_distinct_for_intervals` accepts numeric/date/timestamp/interval endpoints but rejected TIME at analysis time. TIME has a natural numeric (nanosecond-of-day) ordering, so it can be bucketed like the other temporal types. ### Does this PR introduce _any_ user-facing change? Yes - `approx_count_distinct_for_intervals` now accepts TIME columns and endpoints. ### How was this patch tested? Extended `ApproxCountDistinctForIntervalsSuite` with TIME endpoints asserting the per-interval approximate distinct counts; the error-message expectations were updated to include TIME. ### Was this patch authored or co-authored using generative AI tooling? Authored with assistance by Claude Opus 4.8. Closes #56934 from yadavay-amzn/SPARK-57847. Authored-by: Anupam Yadav <anupamy030@gmail.com> Signed-off-by: Max Gekk <max.gekk@gmail.com> (cherry picked from commit 1a89a69) Signed-off-by: Max Gekk <max.gekk@gmail.com>
2 participants
What changes were proposed in this pull request?
Adds
TimeTypeto the input types accepted by theapprox_count_distinct_for_intervalsaggregate. TIME values are bucketed by their internal nanosecond-of-dayLongrepresentation, routed through the sameLong -> Doublepath already used forTimestampType/DayTimeIntervalType.Why are the changes needed?
approx_count_distinct_for_intervalsaccepts numeric/date/timestamp/interval endpoints but rejected TIME at analysis time. TIME has a natural numeric (nanosecond-of-day) ordering, so it can be bucketed like the other temporal types.Does this PR introduce any user-facing change?
Yes -
approx_count_distinct_for_intervalsnow accepts TIME columns and endpoints.How was this patch tested?
Extended
ApproxCountDistinctForIntervalsSuitewith TIME endpoints asserting the per-interval approximate distinct counts; the error-message expectations were updated to include TIME.Was this patch authored or co-authored using generative AI tooling?
Authored with assistance by Claude Opus 4.8.